-
Notifications
You must be signed in to change notification settings - Fork 121
Convert pasted files to file paths that work in R #9886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
E2E Tests 🚀 |
@@ -0,0 +1,116 @@ | |||
# (Mostly Windows) File Path Auto-Conversion Feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will presumably remove this file when this feature is done, which may or may not happen in this PR. I.e. might decide to take the win in R documents and handle the console separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we don't commit work-in-progress notes files to the repo -- if you want to preserve these, maybe file an issue and link to it from the relevant code?
token: vscode.CancellationToken | ||
): Promise<vscode.DocumentPasteEdit[] | undefined> { | ||
|
||
const setting = vscode.workspace.getConfiguration('positron.r').get<boolean>('autoConvertFilePaths'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need a setting to allow people to opt out? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need? No. But I think it's fine to leave in until we're confident that this won't get in anyone's way!
/** | ||
* File path utilities for data analysis code. | ||
*/ | ||
namespace paths { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to the positron API to centralize the initial futzing with the clipboard and file URIs, which would be needed to produce usable file paths for any language (e.g. R or Python).
|
The unit test failures re: UNC paths are due to the fact that CI is running on ubuntu and the positron/src/vs/base/common/extpath.ts Lines 122 to 125 in 13c48a0
|
0e76a47
to
aaf9362
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a few nits. this worked well in my testing!
Is there a good reason we shouldn't also do this for Python? (seems like it'd be almost identical other than the array syntax)
token: vscode.CancellationToken | ||
): Promise<vscode.DocumentPasteEdit[] | undefined> { | ||
|
||
const setting = vscode.workspace.getConfiguration('positron.r').get<boolean>('autoConvertFilePaths'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need? No. But I think it's fine to leave in until we're confident that this won't get in anyone's way!
// to get that back. | ||
return [{ | ||
insertText, | ||
title: 'Insert file path(s)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this text be localized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, I don't think this text is ever surfaced 🫠 At some point, when this PR "did more" but also had a weaker design, this text was surfaced (in a "Paste As" sort of experience). I will rationalize this bit one way or another before I merge.
} | ||
|
||
/** | ||
* Extract file paths from clipboard for use in data analysis code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there's no need to specify "for use in data analysis code?" This API is very generic
} | ||
|
||
// Err on the side of caution and skip conversion entirely if ANY paths are | ||
// UNC paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If UNC paths are used it looks like they don't get escaped at all, but are still pasted. Would be nice if this worked but seems like it could be hard!
@@ -0,0 +1,116 @@ | |||
# (Mostly Windows) File Path Auto-Conversion Feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we don't commit work-in-progress notes files to the repo -- if you want to preserve these, maybe file an issue and link to it from the relevant code?
On windows, Claude Code basically constantly shows: PostToolUse:Edit [$CLAUDE_PROJECT_DIR/.claude/hooks/format-hook.sh] failed with non-blocking status code 1: '$CLAUDE_PROJECT_DIR' is not recognized as an internal or external command, operable program or batch file.
638a342
to
3204f08
Compare
(Partially) addresses #8393
RStudio users miss the feature where, if you have one or more files on the clipboard (and I really mean files), and you paste into an R context (an R script or the console), you get a quoted path, with
\
converted to/
.In terms of 👍, this issue is ranked fourth among all issues with the
os-windows
label.This PR implements this paste magic for R files, but not (yet?) for the R console. I note that it is also not working (yet?) for R chunks of
.qmd
or.Rmd
documents.file-paste.mp4
I also still want to make the path relative, e.g.
the current workspace root or user's home directory, if possible (as RStudio does)now implemented. Ironically I haven't test driven this on macOS, because I'm still limping along on my Windows machine but that will change soon.I'm looking for:
positron/src/vs/workbench/contrib/positronConsole/browser/positronConsole.contribution.ts
Lines 91 to 96 in c763d3e
Release Notes
New Features
Bug Fixes
QA Notes